Conversation
9239acc to
f431eca
Compare
7dd10a9 to
f001bd7
Compare
Extract static dependency preparation into a reusable builder and wire CI to
reuse downloaded archives across jobs/platforms. This reduces duplicated build
logic, improves maintainability, and speeds up wheel/sdist workflows.
- Introduce `build_support/lib_xmlsec_dependency_builder.py`:
- Centralize dependency version resolution, source/archive download, extract,
and build steps for OpenSSL, zlib, libiconv, libxml2, libxslt, and xmlsec1.
- Support both Unix and Windows dependency preparation paths.
- Preserve cross-compilation handling via `CrossCompileInfo` (now a dataclass).
- Expose resolved library versions for callers.
- Add `build_libs_xmlsec.py` CLI:
- Provide a standalone entrypoint to prepare dependencies.
- Support `--download-only`, custom `--libs-dir`, custom `--buildroot`,
and target platform/plat-name overrides.
- Refactor `build_support/static_build.py`:
- Delegate dependency preparation to `LibXmlsecDependencyBuilder`.
- Keep extension configuration focused on platform-specific compiler/linker
flags and include/lib wiring.
- Preserve static-link behavior while removing duplicated dependency logic.
- Update `build_support/build_ext.py`:
- Initialize build flags (`PYXMLSEC_ENABLE_DEBUG`, `PYXMLSEC_STATIC_DEPS`,
`PYXMLSEC_OPTIMIZE_SIZE`) in `__init__`.
- Keep build flow unchanged, but use the refactored static helper path.
- Modernize packaging metadata:
- Move project metadata from `setup.py` into PEP 621 fields in
`pyproject.toml` (`[project]`, `[project.urls]`, `[tool.setuptools]`).
- Simplify `setup.py` to extension setup only.
- Delete legacy `setup.cfg`.
- Relax build-system pins and align build requirements with setuptools_scm>=8.
- Bump `ruff[format]` in `requirements-test.txt` to `0.14.4`.
- Add reusable dependency-cache workflow:
- New `.github/workflows/cache_libs.yml` workflow_call job that downloads and
caches `libs/*.{xz,gz,zip}` per OS/arch/version inputs.
- Export cache/version outputs for downstream jobs.
- Validate expected Windows archive filenames.
- Rework wheel/manylinux CI to consume cached libs:
- `manylinux.yml` now depends on `cache_libs`, restores cache, and runs build
+ test inside container via new script
`.github/scripts/manylinux_build_and_test.sh`.
- Script sets `PYTHONPATH` and `PYXMLSEC_LIBS_DIR` explicitly so isolated PEP
517 builds can import local helpers and reuse cached archives.
- `wheels.yml` now depends on `cache_libs`, restores cache before
cibuildwheel, updates action versions, and refreshes matrix generation
(`generate_wheels_matrix`) including ARM Linux runner mapping.
- `sdist.yml` installs `setuptools_scm>=8` during build deps setup.
- Minor workflow hygiene updates:
- Normalize formatting and small ordering/conditional tweaks in
`linuxbrew.yml` and `macosx.yml`.
f001bd7 to
f1d8dab
Compare
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Python packaging configuration by migrating from setup.py/setup.cfg to PEP 621-compliant pyproject.toml, refactors build tooling into a reusable dependency builder, and introduces a workflow caching system. Despite the title "Temporary disable all workflows except wheels," all workflows remain active.
Changes:
- Migrated package metadata from setup.py/setup.cfg to pyproject.toml using PEP 621 standards
- Refactored static dependency building into a standalone LibXmlsecDependencyBuilder class with CLI tool
- Added cache_libs.yml workflow to pre-download and cache library dependencies across builds
- Updated GitHub Actions workflows with dependency caching and manylinux containerized builds
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Migrated all package metadata to PEP 621 format; added project URLs pointing to xmlsec org; configured setuptools_scm>=8 |
| setup.py | Simplified to only define extension module; removed all metadata (now in pyproject.toml); changed to relative paths |
| setup.cfg | Completely removed; functionality migrated to pyproject.toml |
| requirements-test.txt | Updated ruff from 0.13.0 to 0.14.4 |
| build_support/static_build.py | Refactored to delegate dependency building to LibXmlsecDependencyBuilder; simplified from 282 to 116 lines |
| build_support/lib_xmlsec_dependency_builder.py | New 401-line module extracting dependency building logic; supports download-only mode; includes platform-specific version defaults |
| build_support/build_ext.py | Moved instance variable initialization to init; added type hints |
| build_libs_xmlsec.py | New CLI tool for downloading/building dependencies outside main build process |
| .github/workflows/cache_libs.yml | New reusable workflow for caching library dependencies across platforms |
| .github/workflows/wheels.yml | Updated actions versions; added cache_libs dependency; switched to ARM runner labels for aarch64 |
| .github/workflows/manylinux.yml | Refactored to use cache_libs workflow; moved build/test into containerized script |
| .github/workflows/sdist.yml | Minor formatting and dependency updates |
| .github/workflows/macosx.yml | Formatting improvements; reordered codecov step |
| .github/workflows/linuxbrew.yml | Formatting improvements only |
| .github/scripts/manylinux_build_and_test.sh | New shell script to run manylinux builds inside containers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| src_root = Path(__file__).parent / 'src' | ||
| sources = [str(path.absolute()) for path in src_root.rglob('*.c')] | ||
| sources = [str(path.relative_to(Path(__file__).parent)) for path in src_root.rglob('*.c')] |
There was a problem hiding this comment.
The sources list now uses relative paths (path.relative_to(Path(file).parent)) instead of absolute paths. While this is generally better practice for distribution, ensure that this change doesn't break any build scenarios where the working directory may differ from the setup.py location, particularly in isolated build environments used by modern build backends.
| sources = [str(path.relative_to(Path(__file__).parent)) for path in src_root.rglob('*.c')] | |
| sources = [str(path.resolve()) for path in src_root.rglob('*.c')] |
| matrix: | ||
| os: | ||
| - "ubuntu-22.04" | ||
| - "ubuntu-22.04-arm" |
There was a problem hiding this comment.
The runner label "ubuntu-22.04-arm" does not exist in GitHub Actions. GitHub provides ARM runners via different labels. For self-hosted ARM64 runners, you would typically use self-hosted runners with custom labels. For GitHub-hosted runners, ARM support is limited. Consider using standard runners or verify that you have self-hosted runners configured with this label.
| - "ubuntu-22.04-arm" | |
| - "ubuntu-22.04-arm64" |
| def _build_openssl(self, env, prefix_arg, cross_compile): | ||
| self.info('Building OpenSSL') | ||
| openssl_dir = next(self.build_libs_dir.glob('openssl-*')) | ||
| openssl_config_cmd = [prefix_arg, 'no-shared', '-fPIC', '--libdir=lib'] |
There was a problem hiding this comment.
The detailed comment explaining why "no-asm" is needed for riscv64 OpenSSL builds has been removed during the refactoring. This workaround addresses a specific linker issue (R_RISCV_JAL relocation failure) that could be encountered in future OpenSSL versions. Consider adding a brief comment explaining this workaround to help future maintainers understand why this platform-specific flag is necessary.
| openssl_config_cmd = [prefix_arg, 'no-shared', '-fPIC', '--libdir=lib'] | |
| openssl_config_cmd = [prefix_arg, 'no-shared', '-fPIC', '--libdir=lib'] | |
| # On riscv64, disable OpenSSL assembly to avoid R_RISCV_JAL relocation | |
| # failures seen with some OpenSSL versions when linking xmlsec. |
| classifiers = [ | ||
| "Development Status :: 5 - Production/Stable", | ||
| "Intended Audience :: Developers", | ||
| "Intended Audience :: System Administrators", | ||
| "Operating System :: OS Independent", | ||
| "Programming Language :: C", | ||
| "Programming Language :: Python :: 3", | ||
| "Programming Language :: Python :: 3.9", | ||
| "Programming Language :: Python :: 3.10", | ||
| "Programming Language :: Python :: 3.11", | ||
| "Programming Language :: Python :: 3.12", | ||
| "Programming Language :: Python :: 3.13", | ||
| "Programming Language :: Python :: 3.14", | ||
| "Topic :: Text Processing :: Markup :: XML", | ||
| "Typing :: Typed" | ||
| ] |
There was a problem hiding this comment.
The classifiers list is missing the "License :: OSI Approved :: MIT License" classifier that is typically included for MIT-licensed projects. This classifier was present in the original setup.py and should be retained for better package discoverability on PyPI.
| self.info(f'Unpacking {archive.name}') | ||
| try: | ||
| with tarfile.open(str(archive)) as tar: | ||
| tar.extractall(path=str(self.build_libs_dir)) |
There was a problem hiding this comment.
The tarfile.extractall() call is potentially unsafe as it doesn't use the 'filter' parameter to prevent path traversal attacks. Starting with Python 3.12, it's recommended to use extractall(path=..., filter='data') to safely extract archives. Consider adding the filter parameter for better security, especially when extracting archives downloaded from external sources.
| tar.extractall(path=str(self.build_libs_dir)) | |
| if sys.version_info >= (3, 12): | |
| tar.extractall(path=str(self.build_libs_dir), filter="data") | |
| else: | |
| tar.extractall(path=str(self.build_libs_dir)) |
| {name = "Oleg Hoefling", email = "oleg.hoefling@gmail.com"}, | ||
| {name = "Amin Solhizadeh", email = "amin.solhizadeh@gmail.com"} | ||
| ] | ||
| license = "MIT" |
There was a problem hiding this comment.
The license field uses a shorthand value "MIT" but should use a SPDX license identifier format for PEP 621 compliance. According to PEP 621, the license field should either be a text string (for non-SPDX licenses) or use {text = "license text"} or {file = "LICENSE"} format. For MIT license, you should use either: license = {text = "MIT"} or license = {file = "LICENSE"}. This ensures better compatibility with modern packaging tools.
| license = "MIT" | |
| license = {text = "MIT"} |
| - "ubuntu-22.04-arm" | ||
| - "macos-latest" | ||
| - "windows-2022" | ||
| - "windows-11-arm" |
There was a problem hiding this comment.
The runner label "windows-11-arm" does not exist in GitHub Actions. GitHub does not provide Windows ARM64 runners as hosted runners. If you need Windows ARM64 builds, you would need to set up self-hosted runners with custom labels. Consider removing this from the matrix or verifying you have self-hosted runners configured with this label.
| - "windows-11-arm" |
| cibuildwheel --print-build-identifiers --platform linux \ | ||
| | jq -nRc '{"only": inputs, "os": "ubuntu-latest"}' \ | ||
| | jq -nRc '{"only": inputs, "os": "ubuntu-22.04"}' \ | ||
| | sed -e '/aarch64/s|ubuntu-22.04|ubuntu-22.04-arm|' \ |
There was a problem hiding this comment.
The runner label "ubuntu-22.04-arm" does not exist in GitHub Actions. This sed command attempts to modify the matrix to use this non-existent runner for aarch64 builds. GitHub does not provide hosted ARM runners with this label. For ARM64 builds, you typically need self-hosted runners or use QEMU emulation on standard x86_64 runners (which is what cibuildwheel normally does). Consider removing this sed command and letting all Linux builds run on ubuntu-22.04.
| | sed -e '/aarch64/s|ubuntu-22.04|ubuntu-22.04-arm|' \ |
| cibuildwheel --print-build-identifiers --platform linux \ | ||
| | jq -nRc '{"only": inputs, "os": "ubuntu-latest"}' \ | ||
| | jq -nRc '{"only": inputs, "os": "ubuntu-22.04"}' \ | ||
| | sed -e '/aarch64/s|ubuntu-22.04|ubuntu-22.04-arm|' \ |
There was a problem hiding this comment.
This line attempts to build Windows ARM64 wheels on a runner labeled "windows-11-arm", which does not exist in GitHub Actions. GitHub does not provide Windows ARM64 hosted runners. This will cause the workflow matrix generation to include an impossible build target. Consider removing the Windows ARM64 build from the matrix or verifying you have self-hosted runners configured with this label.
No description provided.